feat: Added basic support for long argument strings#25
Conversation
mkrl
left a comment
mkrl
left a comment
There was a problem hiding this comment.
I would say parsing it this way overcomplicates things a little and doesn't really scale up that good.
We could probably use something like this:
- Get the actual command from the input: single word before the first space
- Use regex to get the rest of the arguments from what's left: something like
("[^"]+"|[^\s"]+)would work great.
That will further reduce the code size and will look more tidy IMO.
|
Thanks for the suggestion! That is a much cleaner way of doing this. As far as I can see this is now handling all but one of the edge cases I could think of. Double and single quotes are handled. Double quotes can be inside single quotes and vice versa. Multiple quoted arguments are handled, as well as a mix of quoted arguments and single-word arguments. The only edge case I could think of that this doesn't handle is nested quotes of the same type, ie |
mkrl
left a comment
mkrl
left a comment
There was a problem hiding this comment.
Great job! 👍
The only edge case I could find is words that contain quote marks without the closing pair will be correctly parsed, but the quotation marks will disappear from the output echo "foo => foo.
I don't think it's a big deal, perhaps it could be addressed in a separate PR.
I have left a suggestion that I think will make it even better, but feel free to skip it if you don't feel like using it.
| const splitCommand = cmd.split(' ') | ||
| const command = splitCommand[0] | ||
| const commandArguments = splitCommand.slice(1) | ||
|
|
||
| const commandArgumentsString = splitCommand.slice(1).join(' ') | ||
| const argRegex = /('[^']+'|"[^"]+"|[^\s'"]+)/g | ||
| const argMatches = commandArgumentsString.match(argRegex) | ||
| const commandArguments = (argMatches === null ? [] : argMatches) | ||
| .map(element => element.replace(/(^['"]|['"]$)/g, '')) |
There was a problem hiding this comment.
That's great!
The only thing that just came to my mind is - we could make this even smaller by getting rid of some variables that have only been used once and doing a shorter array destructing.
I have also included a tiny change to avoid .map pass over an empty array.
const [command, ...args] = cmd.split(' ')
const argMatches = args.join(' ').match(/('[^']+'|"[^"]+"|[^\s'"]+)/g)
const commandArguments = argMatches === null
? []
: argMatches
.map(element => element.replace(/(^['"]|['"]$)/g, ''))(can't make a suggestion because it includes removed lines)
The size difference is ~10 bytes in the final build, but every byte counts :)
There was a problem hiding this comment.
Changes made as suggested, thanks for the input! I didn't know you could do that kind of array destructing in js, that's a neat trick.
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Description
First draft at addressing issue #22 .
Looks for an opening quotation mark in the first index of the command argument array. If one is found, the matching closing quotation is found and the enclosing string is joined into a single long argument. The input
echo "test this"printstest this.Handles quotations around a single word as well (e.g.
echo "test").Does not handle single quotes ('') at all.
Does not handle multiple instances of long arguments
How is this looking so far? Should this be placed into a its own function in the same file? Should I try to implement single quotes and multiple long arguments before this is merged?
Fixes #22 (issue)
Pre-review checklist: